Skip to content

Remove unnecessary join when filtering on relationship id #3922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove unnecessary join when filtering on relationship id #3922

wants to merge 1 commit into from

Conversation

bukajsytlos
Copy link
Contributor

Closes #3349

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 18, 2025
@bukajsytlos bukajsytlos force-pushed the issue/3349 branch 6 times, most recently from 6042c1e to 12cfabd Compare June 21, 2025 13:32
bukajsytlos
bukajsytlos commented Jun 21, 2025
View reviewed changes
spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java
// is the attribute of Collection type?
boolean isPluralAttribute = model instanceof PluralAttribute;

if (propertyPathModel == null && isPluralAttribute) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propertyPathModel being null is no longer issue in hibernate

@mp911de mp911de self-assigned this Jun 23, 2025
@mp911de
Copy link
Member

mp911de commented Jul 2, 2025

Thanks a lot. Does this optimization also work for composite id's and/or parts of a composite identifier?

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 2, 2025
@bukajsytlos
Copy link
Contributor Author

bukajsytlos commented Jul 5, 2025 •
edited
Loading

It should. I've added some more tests. Unfortunately findByRelationshipPartialIdClass is failing on main branch, but is working on 3.4.x (both hibernate 6.x or 7.x).
To me it looks like hibernate issue, but if you could take a look it would be nice

org.springframework.orm.jpa.JpaSystemException: Error accessing field [private org.springframework.data.jpa.domain.sample.IdClassExampleDepartment org.springframework.data.jpa.domain.sample.IdClassExampleEmployee.department] by reflection for persistent property [org.springframework.data.jpa.domain.sample.IdClassExampleEmployee#department] : org.springframework.data.jpa.domain.sample.IdClassExampleEmployeePK@3e1

Test also fails without my changes to query utils

@mp911de
Copy link
Member

mp911de commented Jul 14, 2025

Looks like a Hibernate issue. The query is:

SELECT r FROM org.springframework.data.jpa.domain.sample.ReferencingIdClassExampleEmployee r LEFT JOIN r.employee e WHERE e.empId = :employeeId

Exception (it's a masked ClassCastException):

java.lang.ClassCastException: Cannot cast org.springframework.data.jpa.domain.sample.IdClassExampleEmployeePK to org.springframework.data.jpa.domain.sample.IdClassExampleEmployee
	at java.base/java.lang.Class.cast(Class.java:3523)
	at java.base/jdk.internal.reflect.MethodHandleObjectFieldAccessorImpl.get(MethodHandleObjectFieldAccessorImpl.java:57)
	at java.base/java.lang.reflect.Field.get(Field.java:440)
	at org.hibernate.property.access.spi.GetterFieldImpl.get(GetterFieldImpl.java:49)
	at org.hibernate.metamodel.mapping.internal.AbstractEmbeddableMapping.getValue(AbstractEmbeddableMapping.java:111)
	at org.hibernate.sql.results.graph.embeddable.internal.NonAggregatedIdentifierMappingInitializer.resolveInstanceSubInitializers(NonAggregatedIdentifierMappingInitializer.java:321)
	at org.hibernate.sql.results.graph.embeddable.internal.NonAggregatedIdentifierMappingInitializer.resolveInstance(NonAggregatedIdentifierMappingInitializer.java:308)
	at org.hibernate.sql.results.graph.embeddable.internal.NonAggregatedIdentifierMappingInitializer.resolveInstance(NonAggregatedIdentifierMappingInitializer.java:45)
	at org.hibernate.sql.results.graph.Initializer.resolveInstance(Initializer.java:151)
	at org.hibernate.sql.results.graph.entity.internal.EntitySelectFetchInitializer.resolveInstance(EntitySelectFetchInitializer.java:186)
	at org.hibernate.sql.results.graph.entity.internal.EntitySelectFetchInitializer.resolveInstance(EntitySelectFetchInitializer.java:42)
	at org.hibernate.sql.results.graph.Initializer.resolveInstance(Initializer.java:151)
	at org.hibernate.sql.results.graph.entity.internal.EntityInitializerImpl.resolveInstanceSubInitializers(EntityInitializerImpl.java:679)
	at org.hibernate.sql.results.graph.entity.internal.EntityInitializerImpl.resolveKey(EntityInitializerImpl.java:606)
	at org.hibernate.sql.results.graph.entity.internal.EntityInitializerImpl.resolveKey(EntityInitializerImpl.java:469)
	at org.hibernate.sql.results.graph.entity.internal.EntityInitializerImpl.resolveKey(EntityInitializerImpl.java:95)
	at org.hibernate.sql.results.internal.StandardRowReader.coordinateInitializers(StandardRowReader.java:230)
	at org.hibernate.sql.results.internal.StandardRowReader.readRow(StandardRowReader.java:136)
	at org.hibernate.sql.results.spi.ListResultsConsumer.read(ListResultsConsumer.java:245)
	at org.hibernate.sql.results.spi.ListResultsConsumer.readRows(ListResultsConsumer.java:235)
	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:166)
	at org.hibernate.sql.results.spi.ListResultsConsumer.consume(ListResultsConsumer.java:33)
	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.doExecuteQuery(JdbcSelectExecutorStandardImpl.java:211)
	at org.hibernate.sql.exec.internal.JdbcSelectExecutorStandardImpl.executeQuery(JdbcSelectExecutorStandardImpl.java:106)
	at org.hibernate.sql.exec.spi.JdbcSelectExecutor.executeQuery(JdbcSelectExecutor.java:64)
	at org.hibernate.sql.exec.spi.JdbcSelectExecutor.list(JdbcSelectExecutor.java:138)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.lambda$new$1(ConcreteSqmSelectQueryPlan.java:145)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.withCacheableSqmInterpretation(ConcreteSqmSelectQueryPlan.java:456)
	at org.hibernate.query.sqm.internal.ConcreteSqmSelectQueryPlan.performList(ConcreteSqmSelectQueryPlan.java:389)
	at org.hibernate.query.sqm.internal.QuerySqmImpl.doList(QuerySqmImpl.java:401)
	at org.hibernate.query.spi.AbstractSelectionQuery.list(AbstractSelectionQuery.java:144)
	at org.hibernate.query.Query.getResultList(Query.java:121)
	at org.springframework.data.jpa.repository.query.JpaQueryExecution$CollectionExecution.doExecute(JpaQueryExecution.java:130)
	at org.springframework.data.jpa.repository.query.JpaQueryExecution.execute(JpaQueryExecution.java:97)
	at org.springframework.data.jpa.repository.query.AbstractJpaQuery.doExecute(AbstractJpaQuery.java:164)
	at org.springframework.data.jpa.repository.query.AbstractJpaQuery.execute(AbstractJpaQuery.java:154)
	at org.springframework.data.repository.core.support.RepositoryMethodInvoker.doInvoke(RepositoryMethodInvoker.java:169)
	at org.springframework.data.repository.core.support.RepositoryMethodInvoker.invoke(RepositoryMethodInvoker.java:158)
	at org.springframework.data.repository.core.support.QueryExecutorMethodInterceptor.doInvoke(QueryExecutorMethodInterceptor.java:167)
	at org.springframework.data.repository.core.support.QueryExecutorMethodInterceptor.invoke(QueryExecutorMethodInterceptor.java:146)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
	at org.springframework.data.projection.DefaultMethodInvokingMethodInterceptor.invoke(DefaultMethodInvokingMethodInterceptor.java:69)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
	at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:369)
	at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:118)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
	at org.springframework.dao.support.PersistenceExceptionTranslationInterceptor.invoke(PersistenceExceptionTranslationInterceptor.java:135)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
	at org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor$CrudMethodMetadataPopulatingMethodInterceptor.invoke(CrudMethodMetadataPostProcessor.java:138)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:179)
	at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:222)
	at jdk.proxy2/jdk.proxy2.$Proxy168.findByEmployee_EmpId(Unknown Source)
	at org.springframework.data.jpa.repository.RepositoryWithCompositeKeyTests.findByRelationshipPartialIdClass(RepositoryWithCompositeKeyTests.java:439)

I've reproduced the issue just using EntityManager. Interestingly, the issue only surfaces when running the query in the same transaction, outside of the transaction, the issue no longer seems to happen. Also, the reproducer catches a different exception, however, it shows the same assignment mismatch. You can find the reproducer and exception message at https://gist.github.com/mp911de/33517cdeedc4fabcc1a6b807976396db.

Also, noteworthy, when introducing equals/hashCode to all types (IdClassExampleEmployee, ReferencingIdClassExampleEmployee) being asserted and flushing/clearing EntityManager, then your test passes.

@bukajsytlos
Copy link
Contributor Author

I am not sure from the last comment whether there is something that I should change to move this PR forward?

@mp911de
Copy link
Member

mp911de commented Jul 17, 2025

If you want, please report the problem to the Hibernate team using code from the reproducer. Hibernate folks are somewhat picky and want folks to use their reproducer templates. In recent reports, the moment the've seen Spring being involved, they backed off immediately and so I am tired filing bug reports there. Rather, I focus on investigating the cause to see whether there is something we can fix on our side (which we can't).

Circling back to the original request, I suggest to wait for a Hibernate fix before we proceed. Shipping a feature where composite Id's cannot be used feels like an incomplete feature from our side.

@mp911de mp911de added the status: blocked An issue that's blocked on an external project change label Jul 17, 2025
@bukajsytlos
Copy link
Contributor Author

Ok I will try to report it using their template, although I feel what you are saying.
Regardless, as I said, this test is failing even without my changes, so I don't see it as blocker.

@mp911de
Copy link
Member

mp911de commented Jul 17, 2025

Technically, it isn't a blocker, but once we're going to advertise the feature it receives more attention and specific cases are going to fail creating a hard time for users wanting to use the feature. Let's get some indication from the Hibernate team how it goes to get a better understanding of how and when we can progress.

@bukajsytlos
Copy link
Contributor Author

https://hibernate.atlassian.net/browse/HHH-19625

@bukajsytlos
Copy link
Contributor Author

Hibernate issue is fixed in versions 6.6.24 and 7.0.9.

@mp911de mp911de mentioned this pull request Aug 11, 2025
Closed
@mp911de mp911de removed the status: blocked An issue that's blocked on an external project change label Aug 13, 2025
@mp911de mp911de added this to the 4.0 M5 (2025.1.0) milestone Aug 13, 2025
mp911de pushed a commit that referenced this pull request Aug 13, 2025
@bukajsytlos @mp911de
Remove unnecessary join when filtering on relationship id.
054b7f4 
We now no longer create a join for query property paths that point to an identifier of referenced entities to optimize query creation.

Closes #3349
Original pull request: #3922
See also: #3970

Signed-off-by: Jakub Soltys <[email protected]>
mp911de added a commit that referenced this pull request Aug 13, 2025
@mp911de
Polishing.
1e49675 
Introduce ExpressionFactory to reduce code duplications. Unify JpqlUtils and QueryUtils expression creation to reduce code duplications. Add Eclipselink tests.

Many thanks to @academey for design ideas.

See #3349
Original pull request: #3922
See also: #3970
@mp911de
Copy link
Member

mp911de commented Aug 13, 2025 •
edited
Loading

Thank you for your contribution. That's merged and polished now. I left a comment at #3970 to explain how we went about two contributions for the same topic and why we decided to use this PR as base for the merge.

@mp911de mp911de closed this Aug 13, 2025
@bukajsytlos
Copy link
Contributor Author

Thanks!

PS: I guess you meant #3970 :)

@mp911de
Copy link
Member

mp911de commented Aug 14, 2025

Indeed, good catch, updated the reference.

academey added a commit to academey/spring-data-jpa that referenced this pull request Aug 16, 2025
@academey
Refactor @Any annotation support to align with project conventions
6e28117 
- Move tests from separate file to QueryUtilsIntegrationTests
- Move isAnyAnnotatedProperty method to appropriate location
- Follow PR spring-projects#3922 pattern for test structure
- Simplify code and remove unnecessary comments

Signed-off-by: Hyunjoon Park <[email protected]>
Signed-off-by: academey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mp911de mp911de

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
4.0 M5 (2025.1.0)
Development

Successfully merging this pull request may close these issues.

Skip JOIN for predicates that compare the primary key of a @ManyToOne relationship
3 participants
@bukajsytlos @mp911de @spring-projects-issues